feat: allow removing a pointer from a field #7518

cjbland · 2021-08-24T01:24:01Z

New Pull Request Checklist

I am not disclosing a vulnerability.
I am creating this PR in reference to an issue.

Issue Description

Creates a new field on the pointer type to allow removing (unlink) an object from a pointer field.

Related issue: #7517

Approach

TODOs before merging

Add test cases
Add entry to changelog
Add changes to documentation (guides, repository pages, in-code descriptions)
~~Add security check~~

codecov · 2021-08-24T01:30:39Z

Codecov Report

Merging #7518 (cea7c62) into master (12eb6c8) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head cea7c62 differs from pull request most recent head 4ab49b7. Consider uploading reports for the commit 4ab49b7 to get more accurate results

@@            Coverage Diff             @@
##           master    #7518      +/-   ##
==========================================
- Coverage   93.97%   93.92%   -0.06%     
==========================================
  Files         181      181              
  Lines       13354    13275      -79     
==========================================
- Hits        12550    12468      -82     
- Misses        804      807       +3

Impacted Files	Coverage Δ
src/GraphQL/loaders/parseClassTypes.js	`94.23% <ø> (ø)`
src/GraphQL/transformers/mutation.js	`93.61% <100.00%> (+0.13%)`	⬆️
src/batch.js	`91.37% <0.00%> (-1.73%)`	⬇️
src/Adapters/Files/GridFSBucketAdapter.js	`79.50% <0.00%> (-0.82%)`	⬇️
src/Security/CheckGroups/CheckGroupServerConfig.js	`95.23% <0.00%> (-0.42%)`	⬇️
src/triggers.js	`94.95% <0.00%> (-0.41%)`	⬇️
src/Controllers/LiveQueryController.js	`96.42% <0.00%> (-0.35%)`	⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`92.37% <0.00%> (-0.32%)`	⬇️
src/GraphQL/ParseGraphQLSchema.js	`96.96% <0.00%> (-0.23%)`	⬇️
src/LiveQuery/ParseLiveQueryServer.js	`95.50% <0.00%> (-0.19%)`	⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12eb6c8...4ab49b7. Read the comment docs.

mtrezza · 2021-08-24T07:21:08Z

Thanks for this PR!

Please make sure to check the "I'm not disclosing a vulnerability" box in the template and keep in mind that if you encounter any vulnerability to not discuss or include it as part of this PR.

cjbland · 2021-08-24T11:35:52Z

Will do, I also plan on updating the documentation, I just ran out of time yesterday. I will add it in today.

mtrezza · 2021-08-24T13:34:41Z

Great! Let us know if you need any guidance.

cjbland · 2021-08-26T23:48:30Z

I checked in the CHANGELOG.md changes and I've forked the docs so I can add documentation on how to interact with objects in GraphQL. Please let me know if there is anything else you need from me for this PR.

Moumouls

Use op delete

src/GraphQL/transformers/mutation.js

mtrezza · 2021-10-14T23:25:36Z

@cjbland could you take a look at this PR? it would be great if we could get this ready for merge.

parse-github-assistant · 2021-10-14T23:26:03Z

The label type:improvement cannot be used here.

cjbland · 2021-10-16T17:37:05Z

Just pushed some changes, let me know if that's what you're looking for.

parse-github-assistant · 2021-10-16T21:06:57Z

Thanks for opening this pull request!

🎉 We are excited about your hands-on contribution!

CHANGELOG.md

Moumouls

LGTM

mtrezza · 2021-10-18T08:03:30Z

@cjbland could you look at the failing tests?

cjbland · 2021-10-18T18:30:36Z

Yes, sure thing; give me a couple of days to circle back to it.

Moumouls

@cjbland @mtrezza here i think we have a deeper issue on the whole GraphQL API. My team also start to encounter many issues with the current implementation of aField: null on the the GraphQL API.

I think we need here to convert on the graphql layer all null to {__op: "Delete"}, since null in the database will not resolve on {aField: { exists: false}}.

We have the same null handling issue, on File, Pointer and all other field. Currently developers using the GraphQL API do not have any solution (without cloud code) to completly unset a filed from the Parse Object.

I'll push a PR that convert all null value on mutation to {__op: "Delete"}

Also the null to opDelete implementation will be easier for developers to use and will avoid new interfaces like unlink on File. Pointer and all other fields.

PS: Currently my team need to write some cloud code functions to correctly convert null to undefined, via request.object.unset("aField"), it looks like tedious, hard to maintain and expose the developer to weird bugs if some fields are not checked in the cloud function.

Moumouls · 2021-10-19T09:23:26Z

@mtrezza based on my comment #7518 (review)

I'll suggest to close this PR and i'll push a global fix for the whole graphqh API.

Issue: #7648

mtrezza · 2021-10-19T10:45:50Z

I'll push a PR that convert all null value on mutation to {__op: "Delete"}

A null value is not the same as undefined and a database usually differentiates between null values and non-existent fields. For example, in the JS SDK, there is a distinction between obj.set("key", null); and obj.unset("key");. Not allowing this distinction in the GraphQL endpoint means that GraphQL would behave differently than other Parse SDKs and setting a field to null would produce a result that is different from what a developer expects. I think the approach in your previous comment makes more sense. Also see parse-community/parse-dashboard#1721 (comment).

Looking at the GraphQL spec it seems you are correct.

spec/ParseGraphQLServer.spec.js

Moumouls · 2021-10-19T17:57:57Z

Here i can suggest to close this one since #7649 was opened

@mtrezza ok for you ?

mtrezza · 2021-10-19T19:21:22Z

@cjbland does the approach in #7649 work for your use case?

Moumouls · 2021-10-19T19:44:19Z

@mtrezza i added the test that cover the use case of @cjbland on my PR to support pointer removal.

mtrezza · 2021-11-18T22:51:39Z

Thanks @Moumouls so I'm closing this in favor of #7649.

parse-community#7517: Allow removing a pointer from a field

0c84df8

Added entry referencing fix

cea7c62

Moumouls suggested changes Oct 9, 2021

View reviewed changes

src/GraphQL/transformers/mutation.js Outdated Show resolved Hide resolved

mtrezza added the type:improvement label Oct 14, 2021

mtrezza removed the type:improvement label Oct 14, 2021

Returning delete operation instead of null, added extra check in test

b70c6a9

mtrezza changed the title ~~#7517: Allow removing a pointer from a field~~ feature: allow removing a pointer from a field Oct 16, 2021

mtrezza changed the title ~~feature: allow removing a pointer from a field~~ feat: allow removing a pointer from a field Oct 16, 2021

mtrezza requested changes Oct 16, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

cjbland and others added 3 commits October 16, 2021 20:13

Merge branch 'master' into feature/7517-clear-pointer-graphql

0465a73

Moved message under unreleased changes section

1e69ecd

Merge branch 'master' into feature/7517-clear-pointer-graphql

23caaa3

mtrezza requested a review from Moumouls October 17, 2021 19:13

Moumouls approved these changes Oct 18, 2021

View reviewed changes

Merge branch 'master' into feature/7517-clear-pointer-graphql

de2efed

Merge branch 'master' into feature/7517-clear-pointer-graphql

4ab49b7

Moumouls suggested changes Oct 19, 2021

View reviewed changes

mtrezza requested changes Oct 19, 2021

View reviewed changes

spec/ParseGraphQLServer.spec.js Show resolved Hide resolved

mtrezza mentioned this pull request Oct 19, 2021

GraphQL: null on GraphQL API layer should be converted to {__op: "Delete"} #7648

Closed

4 tasks

mtrezza closed this Nov 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow removing a pointer from a field #7518

feat: allow removing a pointer from a field #7518

cjbland commented Aug 24, 2021 •

edited by mtrezza

Loading

codecov bot commented Aug 24, 2021 •

edited

Loading

mtrezza commented Aug 24, 2021

cjbland commented Aug 24, 2021

mtrezza commented Aug 24, 2021

cjbland commented Aug 26, 2021

Moumouls left a comment

mtrezza commented Oct 14, 2021

parse-github-assistant bot commented Oct 14, 2021

cjbland commented Oct 16, 2021

parse-github-assistant bot commented Oct 16, 2021 •

edited

Loading

Moumouls left a comment

mtrezza commented Oct 18, 2021

cjbland commented Oct 18, 2021

Moumouls left a comment •

edited

Loading

Moumouls commented Oct 19, 2021 •

edited

Loading

mtrezza commented Oct 19, 2021 •

edited

Loading

Moumouls commented Oct 19, 2021 •

edited

Loading

mtrezza commented Oct 19, 2021

Moumouls commented Oct 19, 2021 •

edited

Loading

mtrezza commented Nov 18, 2021

feat: allow removing a pointer from a field #7518

feat: allow removing a pointer from a field #7518

Conversation

cjbland commented Aug 24, 2021 • edited by mtrezza Loading

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

codecov bot commented Aug 24, 2021 • edited Loading

Codecov Report

mtrezza commented Aug 24, 2021

cjbland commented Aug 24, 2021

mtrezza commented Aug 24, 2021

cjbland commented Aug 26, 2021

Moumouls left a comment

Choose a reason for hiding this comment

mtrezza commented Oct 14, 2021

parse-github-assistant bot commented Oct 14, 2021

cjbland commented Oct 16, 2021

parse-github-assistant bot commented Oct 16, 2021 • edited Loading

Thanks for opening this pull request!

Moumouls left a comment

Choose a reason for hiding this comment

mtrezza commented Oct 18, 2021

cjbland commented Oct 18, 2021

Moumouls left a comment • edited Loading

Choose a reason for hiding this comment

Moumouls commented Oct 19, 2021 • edited Loading

mtrezza commented Oct 19, 2021 • edited Loading

Moumouls commented Oct 19, 2021 • edited Loading

mtrezza commented Oct 19, 2021

Moumouls commented Oct 19, 2021 • edited Loading

mtrezza commented Nov 18, 2021

cjbland commented Aug 24, 2021 •

edited by mtrezza

Loading

codecov bot commented Aug 24, 2021 •

edited

Loading

parse-github-assistant bot commented Oct 16, 2021 •

edited

Loading

Moumouls left a comment •

edited

Loading

Moumouls commented Oct 19, 2021 •

edited

Loading

mtrezza commented Oct 19, 2021 •

edited

Loading

Moumouls commented Oct 19, 2021 •

edited

Loading

Moumouls commented Oct 19, 2021 •

edited

Loading